Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PG17 compatibility: Fix Test Failure in multi_name_lengths multi_create_table_constraints #7726

Merged
merged 1 commit into from
Dec 2, 2024

Conversation

m3hm3t
Copy link
Contributor

@m3hm3t m3hm3t commented Nov 11, 2024

Relevant PG commit:
e59fcbd712c777eb2987d7c9ad542a7e817954ec

CI link https://github.com/citusdata/citus/actions/runs/11844794788

 \c - - :public_worker_1_host :worker_1_port
 SELECT "Constraint", "Definition" FROM table_checks WHERE relid='public.name_lengths_225002'::regclass ORDER BY 1 DESC, 2 DESC;
                            Constraint                            |                                       Definition                                        
 -----------------------------------------------------------------+-----------------------------------------------------------------------------------------
- nl_checky_1234567890123456789012345678901234567_b16df46d_225002 | CHECK (date_col_12345678901234567890123456789012345678901234567890 >= '01-01-2014'::date)
+ nl_checky_1234567890123456789012345678901234567_b16df46d_225002 | CHECK date_col_12345678901234567890123456789012345678901234567890 >= '01-01-2014'::date
 (1 row)

diff -dU10 -w /__w/citus/citus/src/test/regress/expected/multi_create_table_constraints.out /__w/citus/citus/src/test/regress/results/multi_create_table_constraints.out
--- /__w/citus/citus/src/test/regress/expected/multi_create_table_constraints.out.modified	2024-11-05 09:50:21.744669730 +0000
+++ /__w/citus/citus/src/test/regress/results/multi_create_table_constraints.out.modified	2024-11-05 09:50:21.756669718 +0000
@@ -402,22 +402,22 @@
 SELECT "Column", "Type", "Definition" FROM index_attrs WHERE
     relid = 'check_example_partition_col_key_365068'::regclass;
     Column     |  Type   |  Definition   
 ---------------+---------+---------------
  partition_col | integer | partition_col
 (1 row)
 
 SELECT "Constraint", "Definition" FROM table_checks WHERE relid='public.check_example_365068'::regclass;
              Constraint              |            Definition             
 -------------------------------------+-----------------------------------
- check_example_other_col_check       | CHECK (other_col >= 100)
- check_example_other_other_col_check | CHECK (abs(other_other_col) >= 100)
+ check_example_other_col_check       | CHECK other_col >= 100
+ check_example_other_other_col_check | CHECK abs(other_other_col) >= 100
 

@m3hm3t m3hm3t self-assigned this Nov 11, 2024
Copy link

codecov bot commented Nov 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (m3hm3t/pg17_support@6611e3c). Learn more about missing BASE report.

Additional details and impacted files
@@                  Coverage Diff                   @@
##             m3hm3t/pg17_support    #7726   +/-   ##
======================================================
  Coverage                       ?   89.55%           
======================================================
  Files                          ?      274           
  Lines                          ?    59689           
  Branches                       ?     7446           
======================================================
  Hits                           ?    53457           
  Misses                         ?     4089           
  Partials                       ?     2143           

@m3hm3t m3hm3t marked this pull request as ready for review November 11, 2024 20:53
@naisila naisila changed the title Fix Test Failure in multi_name_lengths in PG17 PG17 compatibility: Fix Test Failure in multi_name_lengths Nov 12, 2024
s/\|[[:space:]]*CHECK[[:space:]]*\((date_col_[a-zA-Z0-9_]+[[:space:]]*[>=<]+[[:space:]].*)\)/| CHECK \1/g

# Specifically remove outer parentheses from CHECK constraints for int_col_* columns, ensuring proper formatting.
s/\|[[:space:]]*CHECK[[:space:]]*\((int_col_[a-zA-Z0-9_]+[[:space:]]*[>=<]+[[:space:]].*)\)/| CHECK \1/g
Copy link
Member

@naisila naisila Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that these two normalization lines are specific to the column name, hence they don't take care of the following failure in multi_create_table_constraints:
https://github.com/citusdata/citus/actions/runs/11775359161/attempts/1#summary-32795798881

SELECT "Constraint", "Definition" FROM table_checks WHERE relid='public.check_example_365068'::regclass;
              Constraint              |            Definition             
 -------------------------------------+-----------------------------------
- check_example_other_col_check       | CHECK (other_col >= 100)
- check_example_other_other_col_check | CHECK (abs(other_other_col) >= 100)
+ check_example_other_col_check       | CHECK other_col >= 100
+ check_example_other_other_col_check | CHECK abs(other_other_col) >= 100
 (2 rows)

So we have date_col, int_col, other_col and abs(other_other_col)
I think we can rename these columns to make the normalization rules apply to them?

Example:
date_col_ to column_date
int_col_ to column_int
other_col to column_other
And then have two normalization lines: one for CHECK column_... and one for CHECK abs(...?
There might be a better way, just thoughts from the top of my head. But definitely in this PR we should fix both multi_create_table_constraints and multi_name_lengths tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to combine the rules into a single, more comprehensive one, but it didn't work out. A more general rule could potentially work, but it carries the risk of affecting other tests as well.

Copy link
Member

@naisila naisila Nov 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand.

In that case, instead of 4 normalization rules, how about we look at this the other way around: add the parantheses where they are missing. In this case, .out files won't change at all, instead just a single normalization line will take care of it.

Something like the following should work:

# PG 17 Removes outer parentheses from CHECK constraints
# we add them back for pg15,pg16 compatibility
# e.g. change CHECK other_col >= 100 to CHECK (other_col >= 100)
s/\| CHECK ([a-zA-Z])(.*)/| CHECK \(\1\2\)/g

40c34fd

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion, @naisila. Adding parentheses for PostgreSQL 15 and 16 compatibility does seem like a straightforward solution that would reduce the number of normalization rules and keep the .out files consistent.

However, I'm wondering if we should align with PostgreSQL 17's standards instead, since it's the latest version and likely reflects the future direction. Adapting the tests to follow PostgreSQL 17's format might help reduce the need for version-specific handling as we move forward.

Let me know your thoughts. In the meantime, I'll try out the suggested normalization and update the PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, it would be ideal to go with PG17's output. But normalization lines slow down the tests so in this case it would be more performant to switch 4 normalization lines for 1, especially since the difference between versions doesn't reflect a feature or improvement in PG17 (the difference between versions only reflects a "style" change let's say). Other than performance, 1 normalization line is also cleaner than 4.
When we drop PG16 support, whichever way we choose, we would have to drop the normalization lines anyway. So, in conclusion, I think we can align with pg16/pg15 standards in this case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @m3hm3t is there something else blocking the update of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@naisila If this version is okay, I will change the base branch to release.

@m3hm3t m3hm3t marked this pull request as draft November 14, 2024 10:00
@m3hm3t m3hm3t changed the title PG17 compatibility: Fix Test Failure in multi_name_lengths PG17 compatibility: Fix Test Failure in multi_name_lengths multi_create_table_constraints Nov 14, 2024
@m3hm3t m3hm3t force-pushed the m3hm3t/multi_name_lengths branch 2 times, most recently from 1b3cfdc to 34e3e90 Compare November 15, 2024 08:31
@m3hm3t m3hm3t changed the base branch from naisila/pg17_support to release-13.0 November 15, 2024 08:31
@m3hm3t m3hm3t force-pushed the m3hm3t/multi_name_lengths branch from 34e3e90 to 8b4a0e2 Compare November 15, 2024 08:32
@m3hm3t m3hm3t requested a review from naisila November 15, 2024 09:49
@m3hm3t m3hm3t marked this pull request as ready for review November 15, 2024 09:49
s/\|[[:space:]]*CHECK[[:space:]]*\((date_col_[a-zA-Z0-9_]+[[:space:]]*[>=<]+[[:space:]].*)\)/| CHECK \1/g

# Specifically remove outer parentheses from CHECK constraints for int_col_* columns, ensuring proper formatting.
s/\|[[:space:]]*CHECK[[:space:]]*\((int_col_[a-zA-Z0-9_]+[[:space:]]*[>=<]+[[:space:]].*)\)/| CHECK \1/g
Copy link
Member

@naisila naisila Nov 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand.

In that case, instead of 4 normalization rules, how about we look at this the other way around: add the parantheses where they are missing. In this case, .out files won't change at all, instead just a single normalization line will take care of it.

Something like the following should work:

# PG 17 Removes outer parentheses from CHECK constraints
# we add them back for pg15,pg16 compatibility
# e.g. change CHECK other_col >= 100 to CHECK (other_col >= 100)
s/\| CHECK ([a-zA-Z])(.*)/| CHECK \(\1\2\)/g

40c34fd

@m3hm3t m3hm3t force-pushed the m3hm3t/multi_name_lengths branch 3 times, most recently from 17f932b to aa6380c Compare November 28, 2024 11:56
@m3hm3t m3hm3t changed the base branch from release-13.0 to naisila/pg17_support November 28, 2024 11:57
@m3hm3t m3hm3t changed the base branch from naisila/pg17_support to m3hm3t/pg17_support November 28, 2024 12:18
@m3hm3t m3hm3t force-pushed the m3hm3t/multi_name_lengths branch 2 times, most recently from 68822cc to 8f54fa5 Compare November 28, 2024 12:26
@m3hm3t m3hm3t requested a review from naisila November 28, 2024 14:55
@m3hm3t m3hm3t force-pushed the m3hm3t/pg17_support branch from 6611e3c to 56595dd Compare November 28, 2024 15:01
remove citus-tools

normalization added

normlize update

update

update

update

update

.

update

.

revert some files

update

update

update

update
@naisila naisila force-pushed the m3hm3t/multi_name_lengths branch from 8f54fa5 to 61ac31f Compare December 2, 2024 10:03
@naisila naisila changed the base branch from m3hm3t/pg17_support to release-13.0 December 2, 2024 10:03
@naisila naisila merged commit e9110de into release-13.0 Dec 2, 2024
1 check passed
@naisila naisila deleted the m3hm3t/multi_name_lengths branch December 2, 2024 10:08
colm-mchugh pushed a commit that referenced this pull request Dec 2, 2024
…te_table_constraints (#7726)

PG 17 Removes outer parentheses from CHECK constraints
we add them back for pg15,pg16 compatibility
e.g. change CHECK other_col >= 100 to CHECK (other_col >= 100)

Relevant PG commit:
e59fcbd712c777eb2987d7c9ad542a7e817954ec
postgres/postgres@e59fcbd

CI link https://github.com/citusdata/citus/actions/runs/11844794788

```difft
 SELECT "Constraint", "Definition" FROM table_checks WHERE relid='public.check_example_365068'::regclass;
              Constraint              |            Definition             
 -------------------------------------+-----------------------------------
- check_example_other_col_check       | CHECK (other_col >= 100)
- check_example_other_other_col_check | CHECK (abs(other_other_col) >= 100)
+ check_example_other_col_check       | CHECK other_col >= 100
+ check_example_other_other_col_check | CHECK abs(other_other_col) >= 100
 
```

Co-authored-by: Mehmet YILMAZ <mehmet.yilmaz@microsoft.com>
naisila added a commit that referenced this pull request Dec 24, 2024
This is the final commit that adds
PG17 compatibility with Citus's current capabilities.

You can use Citus community, release-13.0 branch, with PG17.1.

---------

Specifically, this commit:

- Enables PG17 in the configure script.

- Adds PG17 tests to CI using test images that have 17.1

- Fixes an upgrade test: see below for details
In `citus_prepare_upgrade()`, don't drop any_value when upgrading from
PG16+, because PG16+ has its own any_value function. Attempting to do so
results in the error seen in [pg16-pg17
upgrade](https://github.com/citusdata/citus/actions/runs/11768444117/job/32778340003?pr=7661):
```
ERROR:  cannot drop function any_value(anyelement) because it is required by the database system
CONTEXT:  SQL statement "DROP AGGREGATE IF EXISTS pg_catalog.any_value(anyelement)"
```
When 16 becomes the minimum supported Postgres version, the drop
statements can be removed.

---------

Several PG17 Compatibility commits have been merged before this final one.
All these subtasks are done #7653

See the list below:

Compilation PR: #7699
Ruleutils PR: #7725
Sister PR for tests: citusdata/the-process#159

Helpful smaller PRs:
- #7714
- #7726
- #7731
- #7732
- #7733
- #7738
- #7745
- #7747
- #7748
- #7749
- #7752
- #7755
- #7757
- #7759
- #7760
- #7761
- #7762
- #7765
- #7766
- #7768
- #7769
- #7771
- #7774
- #7776
- #7780
- #7781
- #7785
- #7788
- #7793
- #7796

---------

Co-authored-by: Colm <colmmchugh@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants